code quality and maintainability for ubshmTransport - #3471
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the clarity and maintainability of the UBRing/UBShmTransport implementation by removing leftover debug logs, renaming CQ-related symbols to “poller” terminology, and renaming time-related gflags to include explicit unit suffixes (_s, _us). It also adds a unit test to assert flag units/descriptions and default values to reduce configuration ambiguity.
Changes:
- Renamed multiple UBRing time-related gflags to include explicit unit suffixes and updated call sites accordingly.
- Renamed CQ-related identifiers to poller terminology and cleaned up polling-mode log strings/variables.
- Added a unit test validating time-flag naming, description units, and default values.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/brpc_ubring_unittest.cpp | Adds gflags-based test asserting time-flag unit suffixes, descriptions, and defaults. |
| src/brpc/ubshm/ub_ring.h | Updates DECLARE_int32 for the renamed flying-IO timeout flag. |
| src/brpc/ubshm/ub_ring.cpp | Renames time flags, removes a local conversion macro, and switches close-timer interval conversion to use USEC_TO_NSEC. |
| src/brpc/ubshm/ub_endpoint.h | Renames CQ socket id/op structures to poller terminology and clarifies comments. |
| src/brpc/ubshm/ub_endpoint.cpp | Applies poller renames throughout and cleans up polling-mode loop variable naming. |
| src/brpc/ubshm/shm/shm_ubs.h | Updates DECLARE_int32 for the renamed flying-IO timeout flag. |
| src/brpc/ubshm/shm/shm_ubs.cpp | Renames flying-IO timeout flag and updates timer interval usage accordingly. |
| src/brpc/ubshm_transport.cpp | Removes leftover debug logs and renames a temporary variable for clarity. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| while (poller->op_queue.Dequeue(op)) { | ||
| if (op.type == CqSidOp::ADD) { | ||
| cq_sids.emplace(op); | ||
| } else if (op.type == CqSidOp::REMOVE) { | ||
| cq_sids.erase(op); | ||
|
|
||
| } else if (op.type == CqSidOp::MOD) { | ||
| cq_sids.erase(op); | ||
| cq_sids.emplace(op); | ||
| if (op.type == PollerSidOp::ADD) { | ||
| poller_sids.emplace(op); | ||
| } else if (op.type == PollerSidOp::REMOVE) { |
There was a problem hiding this comment.
This behavior predates the current PR, which only renames the related CQ identifiers to poller terminology without changing the event-registration logic. The ADD/MOD/REMOVE transitions and event-mask handling will be tracked and addressed separately.
| const uint32_t event_q_timer_interval_ns = | ||
| FLAGS_ub_event_queue_timer_interval_us * USEC_TO_NSEC; | ||
| itimerspec time_spec = { | ||
| .it_interval = {.tv_sec = 0, .tv_nsec = event_q_timer_interval}, | ||
| .it_interval = {.tv_sec = 0, .tv_nsec = event_q_timer_interval_ns}, | ||
| .it_value = {.tv_sec = 0, .tv_nsec = 1} |
There was a problem hiding this comment.
The lack of tv_sec/tv_nsec normalization predates this PR. The current change only makes the existing time unit explicit without changing the timer calculation. Interval validation, time normalization, and boundary tests will be tracked and addressed separately.
|
@wwbmmm Thanks for the review. I’ve checked both comments. They identify pre-existing issues rather than regressions introduced by this PR. They are outside the scope of this cleanup PR and will be addressed separately. |
What problem does this PR solve?
Issue Number: #3463 (Phase 1)
Problem Summary:
Clean up UBRing code and make configuration names and polling terminology clearer.
What is changed and the side effects?
Changed:
_sand_ussuffixes to time-related flags and descriptions.USEC_TO_NSECand add tests for flag units and default values.Side effects:
Check List:
brpc_ubring_unittestpasses.